Skip to content

Fork safe ConcurrentMultiSpanProcessor #4862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
gregoiredx wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fork safe ConcurrentMultiSpanProcessor #4862

gregoiredx wants to merge 1 commit into from
+85 −0

Conversation

@gregoiredx
Copy link

@gregoiredx gregoiredx commented Dec 17, 2025 •
edited
Loading

Description

ConcurrentMultiSpanProcessor is not "fork safe". This MR tries to solve this problem.

Forking a threaded process is not a good idea in the first place, citing the Python doc, see https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods

Note that safely forking a multithreaded process is problematic.

Also, the OTel Python SDK suggest to only initialize the SDK post fork, see https://opentelemetry-python.readthedocs.io/en/latest/examples/fork-process-model/README.html

Nevertheless, on an existing code base, it's not always easy to avoid all forks. So it looks like a good idea to try and make the SDK as fork safe as possible.

Note: BatchProcessor is already relying on register_at_fork used here, see

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/_shared_internal/__init__.py

Line 125 in 102fec2

os.register_at_fork(after_in_child=lambda: weak_reinit()()) # pyright: ignore[reportOptionalCall] pylint: disable=unnecessary-lambda

Fixes #2349

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

See unit test and discussions below.

An equivalent patch is currently running in our stack.

Does This PR Require a Contrib Repo Change?

?

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 17, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: gregoiredx / name: Grégoire Deveaux (ac36ce2)

@gregoiredx gregoiredx force-pushed the fork-safe-concurrent-multi-span-processor branch from 3abcc2e to f98d672 Compare December 17, 2025 13:48
@gregoiredx gregoiredx mentioned this pull request Dec 17, 2025
Open
@gregoiredx gregoiredx marked this pull request as ready for review December 22, 2025 08:50
@gregoiredx gregoiredx requested a review from a team as a code owner December 22, 2025 08:50
@xrmx xrmx moved this to Ready for review in @xrmx's Python PR digest Dec 29, 2025
DylanRussell
DylanRussell reviewed Dec 29, 2025
View reviewed changes
@gregoiredx gregoiredx force-pushed the fork-safe-concurrent-multi-span-processor branch 4 times, most recently from 8e7c298 to 808fe91 Compare January 2, 2026 16:38
@gregoiredx gregoiredx changed the title Make ConcurrentMultiSpanProcessor fork safe Fork safe ConcurrentMultiSpanProcessor Jan 2, 2026
@gregoiredx gregoiredx force-pushed the fork-safe-concurrent-multi-span-processor branch from 808fe91 to 071b2de Compare January 9, 2026 12:33
@gregoiredx gregoiredx force-pushed the fork-safe-concurrent-multi-span-processor branch from 071b2de to ac36ce2 Compare January 9, 2026 12:41
@gregoiredx
Copy link
Author

gregoiredx commented Jan 9, 2026 •
edited
Loading

@DylanRussell thanks a lot for the review.
The first pass of the CI wasn't successful.
I applied two fixes:

There's another test which failed but it doesn't look like it's related to my change, see https://github.com/open-telemetry/opentelemetry-python/actions/runs/20662185256/job/59826483790#step:6:39

Can you relaunch the CI please?

@gregoiredx
Copy link
Author

gregoiredx commented Jan 9, 2026

There's still a CI step failing but I have no idea how I can fix this. It looks like a bug/flakiness in the CI workflow
https://github.com/open-telemetry/opentelemetry-python/actions/runs/20852250476/job/59913535963#step:6:22

Updating https://github.com/open-telemetry/opentelemetry-python.git (2b1e6540d049eb4abcf09d445cec06ae389aa1e4)
  × Failed to download and build `opentelemetry-api @
  │ git+[https://github.com/open-telemetry/opentelemetry-python.git@2b1e6540d049eb4abcf09d445cec06ae389aa1e4#egg=opentelemetry-api&subdirectory=opentelemetry-api`](https://github.com/open-telemetry/opentelemetry-python.git@2b1e6540d049eb4abcf09d445cec06ae389aa1e4#egg=opentelemetry-api&subdirectory=opentelemetry-api%60)
  ├─▶ Git operation failed
  ├─▶ failed to find branch, tag, or commit
  │   `2b1e6540d049eb4abcf09d445cec06ae389aa1e4`

@DylanRussell
Copy link
Contributor

DylanRussell commented Jan 9, 2026

Yeah don't worry about that it's a very common flake that randomly occurs on tests, some sort of race condition I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DylanRussell DylanRussell DylanRussell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

@xrmx's Python PR digest
Status: Ready for review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConcurrentMultiSpanProcessor threadpool should be reset after process forked

2 participants

@gregoiredx @DylanRussell